initial implementation of the groth16 verifier by zz-sol · Pull Request #4 · solana-program/groth16-verifier-program

zz-sol · 2026-09-17T12:18:32Z

#2 got merged/closed because of a forced push. reopen this PR

…uction builders

…fy instruction

… arkworks, registry and malformed-input tests

… tests

… budget notes

samkim-crypto

Okay, some comments from my pass through the program.

samkim-crypto

Starting to dig into the verify library, but some initial comments below.

vadorovsky

Still not fully done with reviewing, but I have a couple of comments for now.

vadorovsky · 2026-09-21T13:46:38Z

Could we get rid of this issue by updating to ark-relations 0.6 (and in general, by updating all the arkworks crates)?

Yeah we used ark 0.5 for the agave and cryptography repo due to potential breaking changes, but I don't think there is a reason to stick with the old version in this repo.

fixed in 82f89fd

vadorovsky · 2026-09-21T13:51:35Z

-
 ## Build and test

 Host toolchain is pinned to stable `1.93.1` in `rust-toolchain.toml`. SBF


Not sure if we should mention Rust versions in README. If you keep it, every time we bump the version, we end up with an additional chore of having to update README.

fixed in cadf68e

vadorovsky · 2026-09-21T16:36:25Z

+    let mut data = Vec::with_capacity(3);
+    data.push(Tag::InitializeStaging as u8);
+    data.extend_from_slice(&num_public_inputs.to_le_bytes());


We could avoid a heap allocation and use an array instead, especially since we are dealing with 3 bytes here. 😄

Suggested change

let mut data = Vec::with_capacity(3);

data.push(Tag::InitializeStaging as u8);

data.extend_from_slice(&num_public_inputs.to_le_bytes());

let [lo, hi] = num_public_inputs.to_le_bytes();

let data = [Tag::InitializeStaging as u8, lo, hi];

fixed in a82c046

vadorovsky · 2026-09-21T22:24:30Z

+        return Err(Groth16ProgramError::IdentityKeyElement.into());
+    }
+
+    let mut input = [0u8; 3 * PAIRING_ELEMENT_SIZE];


Instead of initializing the array with zeros, we could either:

Use https://crates.io/crates/arrayvec - my preference, since it provides safe abstraction over MaybeUninit.

Use MaybeUninit and deal with unsafe operations ourselves.

I can come up with more specific proposal tomorrow.

sounds good

samkim-crypto · 2026-09-22T09:13:53Z

+            acc = g1_add(&acc, ic)?;
+        } else {
+            let term = g1_mul(ic, s)?;
+            acc = g1_add(&acc, &term)?;


Let me know if I am misunderstanding this, but we can skip if scalar is zero, right? Maybe we should do something like

let ic = vk.ic(i + 1); if scalar::is_zero(ic) { continue; } if scalar::is_one(s) { acc = g1_add(&acc, ic)?; } else { let term = g1_mul(ic, s)?; acc = g1_add(&acc, &term)?; }

Sorry I may have misunderstood the point but zero scalars are already skipped in line 67. Is that what you meant? or did you want to change the order of doing zero scalar check and ic increment?

Yeah sorry, my wording might have been unclear. I meant checking whether the IC point is the identity (scalar::is_zero(ic)). Even when s is nonzero, multiplying an identity point contributes nothing, so we can skip that term. The existing is_zero(s) check handles zero scalars. Am I misunderstanding the logic here?

oh thanks! I misread the scalars. fixed in 11b2ae7

samkim-crypto

I think the PR looks fine to me as an initial version. The high-level design makes sense to me. Obviously, there are places that can be optimized, but these can be done in follow-up PRs iteratively.

I'll give an approval when any of the outstanding comments above are all addressed.

vadorovsky · 2026-09-24T15:00:45Z

If you don't mind, I would like to have one more day for reviewing this. Although it's very likely that I will approve.

(If there are any deadlines / you're on rush, I can do a post-merge review.)

zz-sol · 2026-09-24T18:27:54Z

If you don't mind, I would like to have one more day for reviewing this. Although it's very likely that I will approve.

(If there are any deadlines / you're on rush, I can do a post-merge review.)

no hurries at all. take your time. thanks for the review!

vadorovsky

Feel free to merge.

Just one non-blocking question that can be addressed later.

vadorovsky · 2026-09-25T14:48:47Z

+
+    #[inline]
+    pub fn a(&self) -> &'a [u8; G1_SIZE] {
+        fixed(self.bytes, PROOF_A_OFFSET)


The current design choice of this Proof struct is that we keep everything in one contiguous array, and then split it into a(), b(), c() etc. on demand. All these methods use fixed() to split the contiguous array into chunks and handle the size-related error.

How often do we expect to use the "chunking" methods a(), b(), c(), and how often do we expect to use as_bytes()? I feel like we are going to use the "chunking" methods way more often. If that's true, then perhaps we should consider keeping each component as a separate field, and then use wincode zero-copy serialization for as_bytes().

On-chain, Verify calls only a_b() and c(), once each per proof, and never as_bytes(); A ‖ B is kept contiguous so pairing slot 0 is a single copy.

I feel like we are already zero-copying and perhaps we don't want to add wincode (an additional dependency)?

samkim-crypto

LGTM!

zz-sol added 11 commits September 12, 2026 15:03

Initial repository skeleton: workspace, toolchain, Makefile and CI

dc62038

Add solana-groth16-verify: stateless BN254 Groth16 verifier and instr…

95fbbfd

…uction builders

Add solana-groth16-program: Pinocchio verifying-key registry and Veri…

1edd2d6

…fy instruction

Add groth16-convert arkworks bridge and the Mollusk test harness with…

555a5f4

… arkworks, registry and malformed-input tests

Add gnark key and proof parsers, the gnark fixture and its end-to-end…

0dae425

… tests

Add groth16-bench: stage-isolating SBF program, CU breakdown test and…

b105c8f

… budget notes

Clean up: full README, design notes and host coverage target

e2c3d7b

recfactor

f2910ec

Update design.md

61a75be

Update design.md

5f23907

linter

6179aed

zz-sol marked this pull request as ready for review September 17, 2026 12:18

zz-sol mentioned this pull request Sep 17, 2026

initial implementation of the groth16 verifier #2

Merged

samkim-crypto reviewed Sep 17, 2026

View reviewed changes

Comment thread groth16-verify/src/lib.rs

Comment thread groth16-verify/src/lib.rs

zz-sol added 10 commits September 18, 2026 15:26

Merge remote-tracking branch 'upstream'

ace6a9b

Add registration flow utilities and docs

21236dd

improve docs for errors

b58e289

update docs w.r.t. key validation

d895da5

Check PDA address before point validation

b430308

Clarify account-count handling

5575269

Add atomic staging and key immutability tests

bef975b

Move entrypoint into processor module

e35b573

Merge branch 'main' into main

d5ed85f

Reject staging==authority; validate system program

a3f8a0e

vadorovsky self-requested a review September 21, 2026 13:33

vadorovsky reviewed Sep 21, 2026

View reviewed changes

samkim-crypto reviewed Sep 22, 2026

View reviewed changes

zz-sol added 2 commits September 23, 2026 18:39

Validate IC points two per G1_ADD at Publish

3465adf

Build instructions with one allocation each

a82c046

zz-sol added 5 commits September 23, 2026 18:39

Use is_multiple_of for the IC table length check

2aba471

Say where the verifier is allocation-free

74033ec

Rename vk.rs to verifying_key.rs

7494b46

Stop repeating toolchain versions in the README

cadf68e

Move to arkworks 0.6

82f89fd

zz-sol mentioned this pull request Sep 23, 2026

update solana-bn254 API to return array instead of Result<Vec<u8>> #5

Open

zz-sol requested review from samkim-crypto and vadorovsky September 23, 2026 23:00

samkim-crypto reviewed Sep 24, 2026

View reviewed changes

zz-sol added 2 commits September 24, 2026 10:22

Skip MSM terms whose IC point is the identity

11b2ae7

OR-reduce is_zero by words instead of testing bytes

be5962f

vadorovsky approved these changes Sep 25, 2026

View reviewed changes

samkim-crypto approved these changes Sep 27, 2026

View reviewed changes

zz-sol merged commit 8bd06b4 into solana-program:main Sep 28, 2026
25 checks passed


		## Build and test

		Host toolchain is pinned to stable `1.93.1` in `rust-toolchain.toml`. SBF

Conversation

zz-sol commented Sep 17, 2026

Uh oh!

samkim-crypto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samkim-crypto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vadorovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zz-sol Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

samkim-crypto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadorovsky commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zz-sol commented Sep 24, 2026

Uh oh!

vadorovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samkim-crypto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

samkim-crypto left a comment •

edited

Loading

samkim-crypto left a comment •

edited

Loading

zz-sol Sep 23, 2026 •

edited

Loading

samkim-crypto left a comment •

edited

Loading

vadorovsky commented Sep 24, 2026 •

edited

Loading